Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add backup attribute to file resource (close #124) #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add backup attribute to file resource (close #124) #191

wants to merge 1 commit into from
+53 −2

Conversation

hico-horiuchi
Copy link
Contributor

Add backup attribute to file (also remote_file, template and http_request).
This is the number of backups to be kept in /var/itamae/backup (default is 5.)
backup method is based on chef/lib/chef/deprecation/provider/file.rb.
(This attribute is same as file of Chef.)

For example:

file `/tmp/file` do
  content 'Hello New World'
  backup 5
end

(In this case, file is backed up to /var/itamae/backup/tmp/file.itamae-20160114001759.)

@hico-horiuchi hico-horiuchi force-pushed the add-backup-attribute-to-file branch 2 times, most recently from b8ebe41 to 0679caa Compare January 13, 2016 15:39
@hico-horiuchi
Copy link
Contributor Author

/var/itamae/backup/tmp/file.itamae-20160114001759 is better ?
Sorry, I fixed backup path to "/var/itamae/backup/#{attributes.path}" according to Chef.

@hico-horiuchi hico-horiuchi force-pushed the add-backup-attribute-to-file branch 3 times, most recently from af1e2d0 to e40a5c2 Compare January 14, 2016 09:39
@hico-horiuchi
Copy link
Contributor Author

@ryotarai Following error occured in Werker at start vm step.
Could you confirm the error ?

There was an error when attemping to rsync a share folder.
Please inspect the error message below for more info.

Host path: /pipeline/build/spec/integration/
Guest path: /vagrant
Error: ssh: connect to host 104.131.69.157 port 22: Connection timed out
rsync: connection unexpectedly closed (0 bytes received so far) [sender]
rsync error: unexplained error (code 255) at io.c(605) [sender=3.0.9]

ryotarai
ryotarai reviewed Jan 15, 2016
View reviewed changes
lib/itamae/resource/file.rb
@@ -169,6 +176,34 @@ def send_tempfile
f.unlink if f
end
end

def backup
return unless (attributes.backup).kind_of?(FalseClass) ? false : attributes.backup > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the following is simpler

return if !attributes.backup || attributes.backup <= 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll fix it.

@hico-horiuchi hico-horiuchi force-pushed the add-backup-attribute-to-file branch from e40a5c2 to e1359a1 Compare January 17, 2016 07:38
hico-horiuchi
hico-horiuchi reviewed Jan 17, 2016
View reviewed changes
lib/itamae/resource/file.rb
backup_files = run_command(['ls', '-1', backup_directory])
backup_files = backup_files.stdout.chomp.split("\n").select { |f|
f.match(/\A#{basename}.itamae-[0-9]+\z/)
}.reverse
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I use run_command("ls -1 #{::File.join(BACKUP_PATH, attributes.path + '.itamae-*')}") ?
(When I use ['ls', '-1', ::File.join(BACKUP_PATH, attributes.path + '.itamae-*'] , * is escaped to \* and can't execute.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thank you!

@hico-horiuchi hico-horiuchi force-pushed the add-backup-attribute-to-file branch 2 times, most recently from 0d72069 to b9224b9 Compare February 10, 2016 15:25
@hico-horiuchi
Copy link
Contributor Author

@ryotarai I apologize for my late reply.
I fixed following codes. Could you confirm?
(I also rebased commits.)

  • Baked-up files have same permission of source files.
    • run_command(['cp', '-p', attributes.path, backup_path])
  • Directories under /var/lib/itamae are 777 .
    • run_command(['install', '-d', '-m=777', backup_directory])

@ryotarai
Copy link
Member

ryotarai commented Mar 6, 2016

Okay. It is better that run_command is replaced with run_specinfra.

@hico-horiuchi
Copy link
Contributor Author

@ryotarai run_specinfra(:copy_file) doesn't seem to preserve the permission.
So, I use run_command(['cp', '-p', attributes.path, backup_path]) .

And run_specinfra(:change_mode) dosen't include -R option.
So, I use run_command(['install', '-d', '-m=777', backup_directory]) .
(run_specinfra(:create_file_as_directory, backup_directory) + run_command(['chmod', '-R', BACKUP_PATH]) is better?)

@ryotarai
Copy link
Member

As you said, Specinfra doesn't have sufficient commands for this PR but you can send a PR to https://github.com/mizzy/specinfra
Would you do it?

@hico-horiuchi
Copy link
Contributor Author

@ryotarai OK! I try to send PR to Specinfra.

@hico-horiuchi hico-horiuchi mentioned this pull request Mar 18, 2016
Merged
@hico-horiuchi hico-horiuchi force-pushed the add-backup-attribute-to-file branch from b9224b9 to 854962a Compare March 20, 2016 14:11
@hico-horiuchi
Copy link
Contributor Author

@ryotarai I fixed by using Specinfra v2.54.0!

@hico-horiuchi hico-horiuchi force-pushed the add-backup-attribute-to-file branch from 854962a to 02b3d42 Compare April 13, 2016 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hico-horiuchi @ryotarai